Skip to content

feat: PASV Port mapping #549

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

feat: PASV Port mapping #549

wants to merge 10 commits into from

Conversation

fclairamb
Copy link
Owner

No description provided.

@codecov Codecov
Copy link

codecov bot commented Jun 9, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 40.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 86.92%. Comparing base (7accbe1) to head (5de27d0).

Files with missing lines Patch % Lines
driver.go 40.00% 4 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #549      +/-   ##
==========================================
+ Coverage   86.80%   86.92%   +0.11%     
==========================================
  Files          12       12              
  Lines        2350     2356       +6     
==========================================
+ Hits         2040     2048       +8     
+ Misses        235      232       -3     
- Partials       75       76       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@openhands-ai OpenHands AI
Copy link

openhands-ai bot commented Jun 9, 2025

Looks like there are a few issues preventing this PR from being merged!

  • GitHub Actions are failing:
    • Build

If you'd like me to help, just leave a comment, like

@OpenHands please fix the failing actions on PR #549

Feel free to include any additional details that might help me get this PR into a better state.

You can manage your notification settings

cursor[bot]
cursor bot reviewed Jun 15, 2025
View reviewed changes
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Local IP Availability Test Flaw

The TestPASVIPMatch test's 127.0.1.1 availability check is flawed. It attempts to dial from 127.0.1.1 to 127.0.0.1:22 (SSH). This can cause the test to be unnecessarily skipped if port 22 is closed, SSH is not running, or a firewall blocks the connection, even if 127.0.1.1 is properly configured and available for local binding.

transfer_test.go#L1100-L1103

ftpserverlib/transfer_test.go

Lines 1100 to 1103 in eaa9484

testAddr := &net.TCPAddr{IP: net.ParseIP("127.0.1.1"), Port: 0}
if _, err := net.DialTCP("tcp", testAddr, &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 22}); err != nil {
t.Skip("Skipping test: 127.0.1.1 not available. Run 'sudo ifconfig lo0 alias 127.0.1.1 up' to enable this test.")
}

Fix in Cursor

Was this report helpful? Give feedback by reacting with 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cursor cursor[bot] cursor[bot] left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fclairamb @KirCute